Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[css-transitions] Split up with variant, set long timeout, rename manual tests #11057

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[css-transitions] Split up with variant, set long timeout, rename manual tests #11057

wants to merge 3 commits into from

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented May 17, 2018 •
edited by wpt-pr-bot

  • Split up transitions-animatable-properties-01.html with variant and set long timeout.
  • Set long timeout for properties-value-*.
  • Rename many manual tests to have -manual suffix.

Fixes #11046.

@zcorpan
[css-transitions] Split up with variant, set long timeout, rename m…
0bf1b10 
…anual tests

* Split up transitions-animatable-properties-01.html with `variant` and set long timeout.
* Set long timeout for properties-value-*.
* Rename many manual tests to have -manual suffix.

Fixes #11046.
@zcorpan zcorpan added the flaky label May 17, 2018
@zcorpan zcorpan requested a review from csnardi May 17, 2018 21:17
@wpt-pr-bot wpt-pr-bot requested a review from dbaron May 17, 2018 21:17
csnardi
csnardi approved these changes May 17, 2018
View reviewed changes
Copy link
Member

@csnardi csnardi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but you'll need someone else to approve this (I don't have write access unfortunately).

@zcorpan zcorpan requested a review from Ms2ger May 17, 2018 21:28
Ms2ger
Ms2ger approved these changes May 18, 2018
View reviewed changes
css/css-transitions/transitions-animatable-properties-01.html Outdated
var p = document.getElementById("testP");
var style = document.getElementById("newStyles");

var testsIntermediate = [];
var testsEnd = [];

setup({timeout: kTIMEOUT});
subsetTest = location.search ? location.search.substr(1) : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing var?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fixed

css/css-transitions/transitions-animatable-properties-01.html Outdated
@@ -2,8 +2,56 @@
<html>
<!-- Submitted from TestTWF Paris -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove comment while you're here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Ms2ger
Copy link
Contributor

Ms2ger commented May 18, 2018

ERROR:lint:css/css-transitions/transition-delay-002-manual.html:36: Console logging API used (CONSOLE)
ERROR:lint:css/css-transitions/transition-delay-003-manual.html:36: Console logging API used (CONSOLE)

@jugglinmike
Copy link
Contributor

See my comment in gh-11046 regarding the file renaming

@zcorpan
Copy link
Member Author

zcorpan commented May 31, 2018

@gsnedders is the renaming here OK or do we want to do it for everything in css/ at the same time?

@Ms2ger
Copy link
Contributor

Ms2ger commented May 31, 2018

I personally have no objection to renaming tests as we work on them.

@gsnedders
Copy link
Member

@zcorpan Can you change the CSS-COLLIDING-TEST-NAME lint to treat foo and foo-manual as equivalent? (Per #5381 (comment) these can otherwise conflict now.) I think this is just changing the key for test_files[source_file.name].add(path) in lint.py?

I'd missed that the harness had been changed, so yeah, the only question is whether we do it all at once or as we touch things; I'm happy either way. I doubt it makes much difference when they're manual anyway!

@gsnedders gsnedders removed their assignment Sep 3, 2018
@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the zcorpan/css-transitions-disabled branch January 24, 2020 18:08
@gsnedders gsnedders restored the zcorpan/css-transitions-disabled branch January 24, 2020 18:51
@Hexcles Hexcles reopened this Jan 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger Ms2ger approved these changes

@csnardi csnardi csnardi approved these changes

@dbaron dbaron Awaiting requested review from dbaron

Assignees
No one assigned
Labels
css-transitions flaky lint status:needs-changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

/css/css-transitions/* are disabled/flaky in mozilla chromium
7 participants
@zcorpan @Ms2ger @jugglinmike @gsnedders @csnardi @Hexcles @wpt-pr-bot